increase the 252 per-block transaction limit #273
Conversation
LarryRuane
force-pushed
the
2020-05-28-darkside-allow-many-tx
branch
from
6653a43 to
13381f1
Compare
LarryRuane
force-pushed
the
2020-05-28-darkside-allow-many-tx
branch
from
13381f1 to
8c48908
Compare
|
|
||
| if i < txCount { | ||
| return nil, errors.New("parsing block transactions: not enough data") | ||
| } |
There was a problem hiding this comment.
Previously, if the number of transactions actually in the block was less than the txCount (variable-length integer) in the block, the error would be silently ignored. Note that it's okay for there to be extra data, because it gets returned (below). In general, this parsing code allows sequences of blocks concatenated together, but we don't currently depend on that. The caller should check that no data remains.
There was a problem hiding this comment.
"The caller should check that no data remains." should be mentioned in a comment above the function
| }, { | ||
| "df2b03619d441ce3d347e9278d87618e975079d0e235dfb3b3d8271510f707aa", | ||
| "8d2593edfc328fa637b4ac91c7d569ee922bb9a6fda7cea230e92deb3ae4b634", | ||
| }, |
There was a problem hiding this comment.
This is simpler than the old code; the old code "knew" that there were 4 blocks containing 1, 1, 2, 2 transactions respectively. So looping over the blocks, and for each block looping over its transactions, happens to amount to exactly 6 transactions The new code specifically lists the txids in each block. The reason for this change is not just to simplify, it's that now the blocks will gain more transactions as the test runs (repeats of the one or 2 that it already has, see below).
| if numTransactions < 253 { | ||
| fmt.Printf("%02x", numTransactions) | ||
| } else { | ||
| fmt.Printf("%02x%02x%02x", 253, numTransactions%256, numTransactions/256) |
There was a problem hiding this comment.
Using %02x seems a little simpler than hex.EncodeToString(), but the effect is the same. It's important to get the endianness correct here, the least-significant digit comes first (little-endian).
|
|
||
| if i < txCount { | ||
| return nil, errors.New("parsing block transactions: not enough data") | ||
| } |
There was a problem hiding this comment.
"The caller should check that no data remains." should be mentioned in a comment above the function
The darksidewalletd code has a restriction that a block can have at most 252 transactions. This is because the number of transactions in a block is encoded as a variable integer (aka compact integer), and the serialization of 253 or greater requires more than one byte, so it's a little complex and messy, so we haven't bothered. But this doesn't allow us to stress-test the lightwalletd and the wallets under the condition that there are more than 252 transactions in a block. Merging this PR will allow #267 to proceed (creating actual tests). This PR also adds a bit more error detection to block parsing.
I tested this manually:
Then verify that there are 253 transactions in this block (the last "index" should be "253"):
grpcurl -plaintext -d '{"height":1000}' localhost:9067 cash.z.wallet.sdk.rpc.CompactTxStreamer/GetBlockProbably good to try the same with 254 and 255, to explore the edge cases.